-
-
Notifications
You must be signed in to change notification settings - Fork 979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache temporary folder #114
Conversation
testDownloadTerraformSourceIfNecessary(t, canonicalUrl, downloadDir, "# Hello, World") | ||
} | ||
|
||
// TODO re-enable these two tests once this is merged into master, since they are trying to download code from master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up that there are a few test cases here trying to download from the Terragrunt repo itself. Once this is merged to master, a few of these will need to tweaked to work correctly. It's a bit awkward to maintain, but very handy as an integration test to check that the caching works correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger. Nice way of handling tests with git.
@josh-padnick Please review when you have a chance. This caching technique makes a pretty massive difference in usability. For example, for our Jenkins code, which relies on ~8 remote Terraform modules, without the caching, every single Terragrunt command has a ~35 second overhead, most of it spent running |
Just reviewing this now, before I even get into the code, can we make the case that this feature of Terragrunt adds value above and beyond the new "supercharged" terraform init command (CTRL-F for "Features (External)")? Conceptually they seems very similar, though it's not clear to me how much caching they do. |
Good question. I'm not sure until I actually use it, but I'm guessing they download code into the local folder as a way to bootstrap a new project. Our use case is to download code into a tmp folder as a way to deploy an immutable version of your code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall. I think you made good decisions around how to cache the downloads (though see my comment about the case where the content behind the same URL has been altered).
Biggest open item for me is how this relates to terraform init
in Terraform 0.9. But within the context of this PR, LGTM!
return false, nil | ||
} | ||
|
||
currentVersion := encodeSourceVersion(terraformSource.CanonicalSourceURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the definition of encodeSourceVersion()
, you appear to be hashing the URL. But what if the URL is the same but its contents are different? This is the case, for example, when we re-release the same git tag. I believe this is the thought behind the -update
in terraform get -update
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I considered this. In the future, if this is a common case, we could add a terragrunt-update
or similar flag. For now, the trivial workaround is to delete the relevant tmp folder manually. My hope, however, is that this is a relatively rare case.
// (e.g. git::github.com/foo/bar//some-module) identifies the module name, and we always want to download the same | ||
// module name into the same folder (see the encodeSourceName method). We also assume the version of the module is | ||
// stored in the query string (e.g. ref=v0.0.3), so we store the base 64 encoded sha1 of the query string in a | ||
// file called .terragrunt-source-version within S. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/S is the base 64 encoded sha1 has of s/S is the base 64 encoded sha1 of s/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: f937e39
DownloadDir: downloadDir, | ||
VersionFile: versionFile, | ||
}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, lots of nuance here. Nicely handled.
terragruntOptions.Logger.Printf("Downloading Terraform configurations from %s into %s", terraformSource.CanonicalSourceURL, terraformSource.DownloadDir) | ||
|
||
terragruntInitOptions := terragruntOptions.Clone(terragruntOptions.TerragruntConfigPath) | ||
terragruntInitOptions.TerraformCliArgs = []string{"init", terraformSource.CanonicalSourceURL.String(), terraformSource.DownloadDir} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider reviewing hashicorp/terraform#11286 for forward-compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like init
may be changed in a backwards incompatible way to be an interactive command. Not sure I can do much about that for now. One alternative is to try to use go-getter directly to reimplement the download functionality, but I'd rather wait and see if with the next version of Terraform, we can leverage init
with special flags to do what we need.
testDownloadTerraformSourceIfNecessary(t, canonicalUrl, downloadDir, "# Hello, World") | ||
} | ||
|
||
// TODO re-enable these two tests once this is merged into master, since they are trying to download code from master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger. Nice way of handling tests with git.
glide.lock
Outdated
- service/mobileanalytics | ||
- service/cloudfront/sign | ||
- service/dynamodb/dynamodbiface | ||
- service/sqs/sqsiface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this many additional AWS services? Will this bloat the binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah. Stupid glide. All I did was add one damn dependency. Hopefully this new version works, and has far fewer extras: 15ed21e
} | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be able to handle recursive deletes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's designed solely to delete files. I've updated the comment to make that clearer: 931215d
|
||
// The path to a file in DownloadDir that stores the version number of the code | ||
VersionFile string | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful to add a comment on what this struct as a whole represents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added: 6de18ef
|
||
// The path to a file in DownloadDir that stores the version number of the code | ||
VersionFile string | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful to add a comment on what this struct as a whole represents. It took me a minute to figure out this represents a set of "source code" for Terraform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added: 6de18ef
rawSourceUrl, err := getter.Detect(source, canonicalWorkingDir, getter.Detectors) | ||
if err != nil { | ||
return nil, errors.WithStackTrace(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call using go-getter to normalize URLs. Looks like a great library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually the library Terraform/Packer/etc use to download files, including the terraform init
command.
Thanks for the feedback, merging now. |
In #111, I added the ability for Terragrunt to download Terraform files from a given
source
URL. The implementation in that PR downloaded the files to a different temp folder every time. As a result, it also had to runterraform get
andterraform remote config
from scratch every time. All together, this added a significant amount of overhead to every single Terraform command.In this PR, I reuse the same temp folder for a given source URL. The exact caching logic is a bit complicated, as there are different cases to handle for local source URLs vs remote source URLs vs remote source URLs with version numbers (e.g.
ref=v0.0.3
). The result is you only have to download the source code once on your computer. After that, everything is exactly as fast as the normal way of using Terraform.